Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

5082 - Make content-security-policy more strict #5107

Closed
wants to merge 2 commits into from
Closed

5082 - Make content-security-policy more strict #5107

wants to merge 2 commits into from

Conversation

kennsippell
Copy link
Member

@kennsippell kennsippell commented Dec 17, 2018 •
edited
Loading

Description

I believe this is our most tightly constrained CSP without breaking anything. It requires a few "unsafe" configurations:

  1. imgSrc: data: - "developers SHOULD NOT include either 'unsafe-inline', or data: as valid sources in their policies. Both enable XSS attacks by allowing code to be included directly in the document itself; they are best avoided completely." https://www.w3.org/TR/CSP3/#csp-directives
  2. scriptSrc: unsafe-eval - We use eval() about 88 times and use new Function() about 17 times in inbox.js. "AngularJS makes use of this in the $parse service to provide a 30% increase in the speed of evaluating AngularJS expressions." https://docs.angularjs.org/api/ng/directive/ngCsp
  3. styleSrc: unsafe-inline - Our dependency, angular-ui-bootstrap injects about 8 inline style blocks. Here is an example issue where they fix one of them Angular-ui is throwing csp error on uib-popover angular-ui/bootstrap#5470. The risks from unsafe css are reduced by fully constraining imgSrc (not done here) and by avoiding unsafe-eval (not done here).

#5082

Questions for Reviewer

  1. I'm explicitly allowing the inline script for telemetry window.startupTimes - through a whitelisted hash. Another option would be to move that script into its own file and appcache it. As-is, if the content of this script ever changes, the corresponding hash will need to be updated. If we keep it as a hash, I'm unsure of the best method to keep the hash and script in sync (new test, calculate the hash in api, just pray, etc.).
  2. We may want to whitelist a hash for each of the eight style blocks from angular-ui-bootstrap instead of usingunsafe-inline - I didn't do it because I'm not sure they are always the same for all browsers but they probably are and can follow-up if we close on that direction.

Additional considerations

This change also makes the header larger. Including it on all resources (js, css, application/json, etc.) isn't required so one minor performance tweak we might consider here is setting it for Accept: html requests only.

CSP also supports report-uri which can alert us when the security policy is violated (due to bugs or successful attacks). Might be worth opening an issue to enable this down the road.

Testing

I went through some basic workflows in webapp + tried each actions in admin pages. In general though, this has risks of breaking edge-case functionalities or workflows I don't know about. Best to AT across all browsers as there are apparently some differences.

Review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Announced in Changes.md in plain English. Configuration and user documentation on medic-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in Changes.md.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@kennsippell
Copy link
Member Author

@garethbowen This one is ready for review or some high level thoughts on "Questions for Reviewers"

@garethbowen garethbowen self-requested a review January 8, 2019 09:17
@garethbowen
Copy link
Contributor

I'm explicitly allowing the inline script for telemetry window.startupTimes - through a whitelisted hash. Another option would be to move that script into its own file and appcache it. As-is, if the content of this script ever changes, the corresponding hash will need to be updated. If we keep it as a hash, I'm unsure of the best method to keep the hash and script in sync (new test, calculate the hash in api, just pray, etc.).

I think we should put it in its own file. It's a shame, but I think it's inevitable we'll modify it and probably not even notice when it breaks.

We may want to whitelist a hash for each of the eight style blocks from angular-ui-bootstrap instead of usingunsafe-inline - I didn't do it because I'm not sure they are always the same for all browsers but they probably are and can follow-up if we close on that direction.

I think unsafe-inline is fine for now. I'm hoping to get off of bootstrap one day as we're not getting much value from it any more.

garethbowen
garethbowen suggested changes Jan 8, 2019
View reviewed changes
Copy link
Contributor

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work and analysis. I think we should pull out a script for the telemetry startup, but otherwise it's looking good.

@kennsippell
Copy link
Member Author

kennsippell commented Jan 8, 2019 •
edited
Loading

Quick chat with @SCdF and it isn't ideal to move this into its own file. Here is an attempted alternative - an E2E test which confirms that the CSP does not block the execution of any inline scripts via log inspection. Happy to move it into its own file if either of you think this is insufficient to catch regressions. I tried changing the telemetry script and it causes this test to fail.

I also attempted to use browser.executeAsyncScript to get the value of window.startupTimes.start to confirm script execution. But we delete window.startupTimes after the telemetry script records the values, which is good.

Discussing with @SCdF, we agreed to move this telemetry script as the first thing in head to capture time to download/parse the css and manifest files.

@garethbowen
Copy link
Contributor

The e2e test is fine too. I was thinking that as part of the service worker switch we'd download the cache on the login page so we would have the file before the app starts, but either way is fine.

garethbowen
garethbowen approved these changes Jan 8, 2019
View reviewed changes
Copy link
Contributor

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sold!

@kennsippell
Copy link
Member Author

kennsippell commented Jan 8, 2019 •
edited
Loading

True regarding the service worker cache... We could also talk about using Workbox's "offline google analytics" library for some telemetry stuff. https://developers.google.com/web/tools/workbox/modules/workbox-google-analytics

Should I create a follow-up item to track removal of the unsafe-inline for css?

@kennsippell
Copy link
Member Author

kennsippell commented Jan 8, 2019 •
edited
Loading

@garethbowen Tests are failing because I ran them (#5198). Does it work if you re-kick? I'm not sure how to get an honest test run out of Travis...

@kennsippell kennsippell closed this Jan 9, 2019
@kennsippell
Copy link
Member Author

Reopening as #5220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garethbowen garethbowen garethbowen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kennsippell @garethbowen